Skip to content

fix: use Exception instead of BaseException in atomic_json_write#1009

Closed
hiSandog wants to merge 1 commit intoNousResearch:mainfrom
hiSandog:fix/use-exception-instead-of-baseexception
Closed

fix: use Exception instead of BaseException in atomic_json_write#1009
hiSandog wants to merge 1 commit intoNousResearch:mainfrom
hiSandog:fix/use-exception-instead-of-baseexception

Conversation

@hiSandog
Copy link

Summary

This PR fixes a critical issue where BaseException was being caught instead of Exception in the atomic_json_write function.

Problem

Catching BaseException is generally a bad practice because it catches system-exiting exceptions like KeyboardInterrupt and SystemExit, which should typically propagate.

Changes

  • Changed exception handling from BaseException to Exception in atomic_json_write
  • This ensures proper exception handling without catching system-level exceptions

Impact

  • More correct exception handling
  • Prevents accidental suppression of KeyboardInterrupt and SystemExit

- BaseException catches KeyboardInterrupt and SystemExit which is not ideal
- Using Exception allows proper signal handling and graceful shutdown
@teknium1
Copy link
Contributor

Closing this one.

except BaseException is intentional in atomic_json_write(). The handler is not suppressing interrupts or exits; it only cleans up the temp file and then immediately re-raises. That means KeyboardInterrupt/SystemExit still propagate normally.

If we narrow this to except Exception, an interrupt during json.dump() leaves an orphaned *.tmp file behind. I verified that behavior locally by forcing json.dump() to raise KeyboardInterrupt.

atomic_yaml_write() uses the same pattern for the same reason, so this would also make the two atomic write helpers inconsistent.

Thanks for the careful look here, but I don’t think this change is correct for Hermes’ atomic-write cleanup semantics.

@teknium1 teknium1 closed this Mar 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants